Per comment below this would become CgroupAttachFlags::default()

I've updated the fn signature to allow flags to be passed in, and updated doc tests to pass in CgroupAttachFlags::empty(). Let me know if that's what you had in mind.

These aren't just SockOps flags though.
See: https://elixir.bootlin.com/linux/latest/source/include/uapi/linux/bpf.h#L1153

This should probably live in links.rs and be called CgroupAttachFlags.

Good call. Done.

I'd prefer changing the API here to add flags: u32 instead of adding a new funciton.

Sure thing, done.

I think I'd rather break the API here and add flags: CgroupAttachFlags.
This makes it explicit which flags are being used.

Sounds good. I wasn't sure how much of an expanding change you'd prefer. I've modified the attach() fn signature here and for other program types.

This makes it explicit which flags are being used.

I'm in two minds about this. For the Cgroup* and SockOps programs, I think it makes sense to have to pass CgroupAttachFlags to attach() - you're explicitly working with cgroups, so it makes sense to have to think about what's going on with other cgroups.

But for SkMsg SkSkb and LircMode2, I think we should default to no flags like before, and have a separate attach_with_flags() for the cases in which you do care about cgroups.

That's a keen observation. I've updated this change following your advice to extend attach() for specific program types, and adding attach_with_flags() where the developer does not necessarily need to think about cgroups.

Aside: Given all the other Cgroup programs can be attached with bpf_link - see:

I'd wager that this is likely the case for sock_ops programs too. I'll open an issue for that.
#987

👍

This doc comment is now inaccurate

Ah, true. I've switched it to the more general comment of "Program attachment flags." Let me know if you'd like a more descriptive comment.

Have you tested this works as expected?

Please see the Testing comment I've added below. This covers the two flags that have been kept in this PR.

And this one

And this one too

This wouldn't make sense to include anyway since we use FD, not ID

Understood. I've removed these additional flags, since ALLOW_MULTI and ALLOW_OVERRIDE were the focus of my change.

I'm not sure that this should be a bitflag vs a regular enum.

The kernel API is weird and uses bitflags, but say we wanted to add support for
BPF_F_REPLACE to attach, we'd have to provide the old program somehow, so it's
unlikely that we'd use attach() to do it.

I see where you're going. I took a look, and I believe the use of bitflags is the more usable interface in this case. This allows the caller to easily set a combination of flags if they wish. The bitflags API frees the developer from having to do bit manipulation themselves.

If we later choose to add support for more flags, such as BPF_F_REPLACE, BPF_F_BEFORE, and BPF_F_AFTER, then we would add the ability to supply the bpf_attr anonymous struct's three additional fields of replace_bpf_fd, relative_fd, relative_id – those would be separate arguments. We would also add a way to set the BPF_F_ID flag to distinguish between the latter two.

I think the use of bitflags now is inline with where we would want to go.

This allows the caller to easily set a combination of flags if they wish. The bitflags API frees the developer from having to do bit manipulation themselves.

My point is that I suspect we won't ever expose the bitflag to the aya API.

If we later choose to add support for more flags, such as BPF_F_REPLACE, BPF_F_BEFORE, and BPF_F_AFTER, then we would add the ability to supply the bpf_attr anonymous struct's three additional fields of replace_bpf_fd, relative_fd, relative_id – those would be separate arguments.

Exactly, similar to https://docs.aya-rs.dev/aya/programs/xdp/struct.xdp#method.attach_to_link where we use a REPLACE bit, but don't expose it to the API, see https://docs.aya-rs.dev/src/aya/programs/xdp.rs#233

Or in other words: you have to pass (flag, fd_or_id) and it's unlikely that we'll have a single method to do that, but more realistically we'll have attach_replace(old) attach_relative(AttachFlags::Before, fd) etc, where the bitflags become an implementation detail but are not exposed

These are good points. From what I understand, you can also attach a program with both the ALLOW_MULTI and ALLOW_OVERRIDE flags set. A bitflags argument is useful there to allow the caller to specify empty, either flag, or set both flags. However, to keep the Aya API simple, we could go with your idea of an enum, and perhaps with a third value that represents both (ALLOW_MULTI_AND_OVERRIDE...?). If we want to avoid an EMPTY value on the enum, we could include both an attach and attach_with_flags function.

An alternative is to implement the flags argument as a struct with two booleans:

struct CgroupAttachFlags {
    allow_multi: bool,
    allow_override: bool,
}

A caller could then set either flag, both flags, or none. That's probably my favorite option, as it seems clean.

Do any of these options resonate with you?

from https://github.com/torvalds/linux/blob/35dfaad7188cdc043fde31709c796f5a692ba2bd/include/uapi/linux/bpf.h#L1094

 * BPF_F_ALLOW_OVERRIDE: If a sub-cgroup installs some bpf program,
 * the program in this cgroup yields to sub-cgroup program.
 *
 * BPF_F_ALLOW_MULTI: If a sub-cgroup installs some bpf program,
 * that cgroup program gets run in addition to the program in this cgroup.

so it seems like they're exclusive.

Given this and the docs of the other flags, I'm not convinced that at the aya level there's any need for bitflags, and bitflags are less idiomatic than regular enums so we should avoid them when possible.

You're right, good instincts. I've updated this change to use an enum, and included a function scoped to the crate to translate the enum to its BPF value.

This makes it explicit which flags are being used.

I'm in two minds about this. For the Cgroup* and SockOps programs, I think it makes sense to have to pass CgroupAttachFlags to attach() - you're explicitly working with cgroups, so it makes sense to have to think about what's going on with other cgroups.

But for SkMsg SkSkb and LircMode2, I think we should default to no flags like before, and have a separate attach_with_flags() for the cases in which you do care about cgroups.

nit: call this CgroupAttachMode?

Here and everywhere else: attach_with_cgroup_mode?

I'm not 100% convinced on mode, do you have a better name?

Ha, now onto the hard part, naming :)

So far I think you've picked a good name. Others I thought of are CgroupAttach- Strategy or Policy or Cardinality or Rule, they each have pros and cons. ("Strategy" is a bit generic, "Policy" is good but feels more security focused, "Cardinality" is accurate but a bit long, "Rule" I kind of like.)

If we go with any of these, I'm thinking of changing the name of the default enum value, as a mode of None sounds odd. Do these value names work?

pub enum CgroupAttachMode {
	Single,
	AllowMultiple,
	AllowOverride,
}

here and everywhere else: please rename the argument name from flag to mode

Right! Done.

I think this should be impl From<CgroupAttachMode> for u32

Done